Keep Grid scroll position when opening a task. - #70536
Conversation
163c64a to
f6c31aa
Compare
f6c31aa to
7fbf2eb
Compare
bbovenzi
left a comment
There was a problem hiding this comment.
Thanks! Actually I think the single shared parent is an interesting route to go. It could help further reduce re-renders.
But let's not store a scrollTop but instead only try to scroll to a task when there is one selected with something like rowVirtualizer.scrollToIndex(). I think we already have the taskId+runId exist as element ids. Then we have no additional state and its possible to deep-link. Also, the scrollTop approach would easily lose sync with task groups expanding/collapsing.
| const DAG_DETAILS_PATH = /^\/dags\/(?<dagId>[^/]+)/u; | ||
|
|
||
| export const extractDagIdFromPath = (pathname: string): string | undefined => | ||
| DAG_DETAILS_PATH.exec(pathname)?.groups?.dagId; |
There was a problem hiding this comment.
We can just call const { dagId } = useParams();
There was a problem hiding this comment.
@bbovenzi Hi, I looked into the option, rowVirtualizer.scrollToIndex() won't work as we have to choose start, center, end or auto and we cannot get back the exact position for the task as when we clicked.
Is there any alternative would you like to suggest ?
Summary
In the Dag details view, the Grid on the left keeps its own vertical scroll position. Clicking a task that is scrolled below the fold currently snaps the Grid back to the top the first time you do it — losing your place. This PR keeps the scroll position across that navigation, while still resetting to the top when you leave the Dag entirely and come back.
Problem
On a Dag with enough tasks to overflow the Grid, scroll down and click a task that is off the top of the viewport. The list jumps back to
scrollTop = 0(the first task) even though the layout looks unchanged. Clicking a second task (task → task) does not reset the scroll — it stays put. So the reset happens exactly once, on the first click, and it gets more painful the longer the list is (e.g. when task groups are expanded).Screenrecords
Original
2026-07-27.15-34-28.mp4
Current
2026-07-27.16-06-37.mp4
Root cause
The Grid lives inside
DetailsLayout, and every top-level page renders its ownDetailsLayout:dags/:dagId→<Dag>dags/:dagId/tasks/:taskId→<Task>dags/:dagId/runs/:runId→<Run>dags/:dagId/runs/:runId/tasks/:taskId→<TaskInstance><GroupTaskInstance>,<MappedTaskInstance>These are sibling routes in
router.tsx, not nested under a shared layout.<Dag>→<Task>): the matched route element changes, so React unmounts<Dag>(destroying itsDetailsLayout, itsGrid, and the scroll container DOM node) and mounts a fresh<Task>. The new scroll container starts atscrollTop = 0, anduseVirtualizerreads0on mount and renders the top rows.<Task>→<Task>): the matched element type is unchanged; only the:taskIdparam updates. React keeps the instance and its DOM, so the scroll position survives.That asymmetry — cross-element on the first hop, same-element afterwards — is why it resets exactly once. It is not task-group specific; groups just make the list longer so more scroll is lost.
Solution (this PR)
Remember the Grid's
scrollTopacross the remount and restore it, scoped to a single Dag visit.useGridScrollRestorationsavesscrollTop(in a module-scopedMapkeyed bydagId, which survives the remount) on scroll, and restores it once, after the rows exist, in auseLayoutEffect(beforepaint, no flash). Covers both the plain-Grid and the shared Grid + Gantt scroll containers.
useResetGridScrollOnLeave(called once from the always-mountedBaseLayout) clears the store whenever the route is not a Dag detail path (/dags/<id>…). So scroll is kept only within a Dag; leaving tothe Dags list / home / assets and returning starts at the top.
dagId)Files:
useGridScrollRestoration.ts(+ test),Grid.tsx(wire-up), and one line inBaseLayout.tsx(reset-on-leave). Routing and the panel layout are untouched.Alternative considered (not taken)
Make
DetailsLayouta single shared parent route so the Grid mounts once and persists across Dag ↔ Run ↔ Task ↔ TaskInstance (the "correct" React Router structure), with per-page content flowing through the existing<Outlet/>.Rejected as disproportionate: it is a large refactor with real regression risk.
Testing
New unit tests cover:
isDagDetailsPathfor detail vs non-detail paths,clearGridScrollOffsetsdrops every entry, and the hook clears on a non-detail path but keeps offsets while inside a Dag (viaMemoryRouter).Was generative AI tooling used to co-author this PR?
Co-authored-by: [Claude Opus 5] following the guidelines
{pr_number}.significant.rst, in airflow-core/newsfragments. You can add this file in a follow-up commit after the PR is created so you know the PR number.